Fix local plugin loading to preserve OAuth assets #287
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (localPluginPath && isModuleResolutionError(err)) { | ||
| log.warn("failed to load local plugin directly, bundling fallback", { | ||
| plugin, | ||
| error: err.message, | ||
| }) | ||
| const bundledPath = await bundleLocalPlugin(localPluginPath) | ||
| await loadPluginModule(pathToFileURL(bundledPath).href) | ||
| continue |
There was a problem hiding this comment.
The second loadPluginModule call on line 161 is not wrapped in a try-catch block. If the bundled plugin fails to load (for any reason - not just module resolution errors), the error will not be caught, and it will propagate uncaught, bypassing the error logging and hint system below.
This means:
- The error won't be logged with the helpful "Make sure all plugin dependencies are installed" hint
- The entire plugin loading will fail instead of just this plugin
- Debugging will be harder because the error won't go through the standardized error handling
| if (localPluginPath && isModuleResolutionError(err)) { | |
| log.warn("failed to load local plugin directly, bundling fallback", { | |
| plugin, | |
| error: err.message, | |
| }) | |
| const bundledPath = await bundleLocalPlugin(localPluginPath) | |
| await loadPluginModule(pathToFileURL(bundledPath).href) | |
| continue | |
| if (localPluginPath && isModuleResolutionError(err)) { | |
| log.warn("failed to load local plugin directly, bundling fallback", { | |
| plugin, | |
| error: err.message, | |
| }) | |
| try { | |
| const bundledPath = await bundleLocalPlugin(localPluginPath) | |
| await loadPluginModule(pathToFileURL(bundledPath).href) | |
| continue | |
| } catch (bundleErr) { | |
| const bErr = bundleErr as Error | |
| log.error("failed to load bundled plugin", { | |
| plugin, | |
| error: bErr.message, | |
| }) | |
| throw bErr | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/opencode/src/plugin/index.ts
Line: 155:162
Comment:
The second `loadPluginModule` call on line 161 is not wrapped in a try-catch block. If the bundled plugin fails to load (for any reason - not just module resolution errors), the error will not be caught, and it will propagate uncaught, bypassing the error logging and hint system below.
This means:
1. The error won't be logged with the helpful "Make sure all plugin dependencies are installed" hint
2. The entire plugin loading will fail instead of just this plugin
3. Debugging will be harder because the error won't go through the standardized error handling
```suggestion
if (localPluginPath && isModuleResolutionError(err)) {
log.warn("failed to load local plugin directly, bundling fallback", {
plugin,
error: err.message,
})
try {
const bundledPath = await bundleLocalPlugin(localPluginPath)
await loadPluginModule(pathToFileURL(bundledPath).href)
continue
} catch (bundleErr) {
const bErr = bundleErr as Error
log.error("failed to load bundled plugin", {
plugin,
error: bErr.message,
})
throw bErr
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| function isModuleResolutionError(err: Error): boolean { | ||
| return err.message?.includes("Cannot find module") || err.message?.includes("Cannot find package") | ||
| } |
There was a problem hiding this comment.
The isModuleResolutionError function uses string matching to detect module resolution errors. While this works for common cases, it could potentially match false positives where a plugin's error message contains these strings but isn't actually a module resolution error.
Consider also checking the error type or code if available:
| function isModuleResolutionError(err: Error): boolean { | |
| return err.message?.includes("Cannot find module") || err.message?.includes("Cannot find package") | |
| } | |
| function isModuleResolutionError(err: Error): boolean { | |
| return ( | |
| err.message?.includes("Cannot find module") || | |
| err.message?.includes("Cannot find package") || | |
| (err as any).code === "MODULE_NOT_FOUND" | |
| ) | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/opencode/src/plugin/index.ts
Line: 86:88
Comment:
The `isModuleResolutionError` function uses string matching to detect module resolution errors. While this works for common cases, it could potentially match false positives where a plugin's error message contains these strings but isn't actually a module resolution error.
Consider also checking the error type or code if available:
```suggestion
function isModuleResolutionError(err: Error): boolean {
return (
err.message?.includes("Cannot find module") ||
err.message?.includes("Cannot find package") ||
(err as any).code === "MODULE_NOT_FOUND"
)
}
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| // @ts-ignore if you feel adventurous, please fix the typing, make sure to bump the try-counter if you | ||
| // give up. | ||
| // try-counter: 2 | ||
| await fn(input, output) |
There was a problem hiding this comment.
Changing from @ts-expect-error to @ts-ignore makes the type checking less strict. @ts-expect-error will warn if there's no type error, helping catch when the typing issue is fixed. @ts-ignore will silently suppress errors even when the typing is correct.
While both work, @ts-expect-error is generally preferred because it provides better type safety feedback. Was there a specific reason for this change, or should we keep the stricter @ts-expect-error?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/opencode/src/plugin/index.ts
Line: 196:199
Comment:
Changing from `@ts-expect-error` to `@ts-ignore` makes the type checking less strict. `@ts-expect-error` will warn if there's no type error, helping catch when the typing issue is fixed. `@ts-ignore` will silently suppress errors even when the typing is correct.
While both work, `@ts-expect-error` is generally preferred because it provides better type safety feedback. Was there a specific reason for this change, or should we keep the stricter `@ts-expect-error`?
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| const config = await Config.get() | ||
| for (const hook of hooks) { | ||
| // @ts-expect-error this is because we haven't moved plugin to sdk v2 | ||
| // @ts-ignore this is because we haven't moved plugin to sdk v2 |
There was a problem hiding this comment.
Same concern as the previous @ts-ignore comment - @ts-expect-error would be stricter and provide better feedback when the typing is eventually fixed.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/opencode/src/plugin/index.ts
Line: 216:216
Comment:
Same concern as the previous `@ts-ignore` comment - `@ts-expect-error` would be stricter and provide better feedback when the typing is eventually fixed.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Summary
Load local file:// plugins directly to preserve asset-relative paths used by OAuth plugins, with a bundling fallback only when module resolution fails.
Changes
Fixes
Breaking Changes
None
Testing
bun turbo typecheck
Greptile Overview
Greptile Summary
This PR modifies the plugin loading mechanism to load local
file://plugins directly instead of bundling them by default, which preserves asset-relative paths needed for OAuth plugins. The bundling process now only occurs as a fallback when direct module import fails due to missing dependencies.Key Changes
Plugin Loading Strategy: The core change inverts the loading approach - instead of always bundling local plugins first, the code now attempts to load them directly via dynamic import. This preserves the plugin's original file structure and relative paths, which is critical for OAuth plugins that reference HTML assets for callback pages.
Fallback Mechanism: When direct loading fails with a module resolution error (missing dependencies), the code falls back to the original bundling approach, which resolves all dependencies into a single file.
Type Safety Improvements: The PR includes several typecheck fixes across multiple files, adding explicit type annotations and defensive null checks to handle potentially undefined plugin hooks.
Integration Points
The changes integrate cleanly with the existing plugin architecture:
bundleLocalPluginfunction remains intact and is now used conditionallyisModuleResolutionErrorhelper centralizes error detection logicCritical Issue Found
There is a logic error in the error handling where the bundled plugin load attempt (line 161) is not wrapped in a try-catch. If the bundled version fails to load, the error will propagate uncaught, bypassing proper error logging and potentially failing the entire plugin initialization process instead of just logging and skipping the problematic plugin.
Confidence Score: 2/5
Important Files Changed
File Analysis
Sequence Diagram
sequenceDiagram participant Config participant Plugin as Plugin.state() participant Loader as loadPluginModule participant Bundler as bundleLocalPlugin participant Import as dynamic import() Config->>Plugin: Get plugin list loop For each plugin alt Non-local plugin (npm) Plugin->>Plugin: Install via BunProc Plugin->>Import: import(npmPluginUrl) Import-->>Plugin: Module loaded else Local plugin (file://) Plugin->>Plugin: Resolve absolute path Note over Plugin: NEW: Try direct load first Plugin->>Loader: loadPluginModule(fileUrl) Loader->>Import: import(fileUrl) alt Import succeeds Import-->>Loader: Module loaded Loader-->>Plugin: Hooks initialized else Import fails (module error) Import-->>Loader: Error: Cannot find module Loader-->>Plugin: Throw error Note over Plugin: Check if module resolution error Plugin->>Bundler: bundleLocalPlugin(localPath) Bundler->>Bundler: Bun.build() with dependencies Bundler->>Bundler: Copy assets to cache Bundler-->>Plugin: Return bundled path Note over Plugin: ISSUE: Not wrapped in try-catch Plugin->>Loader: loadPluginModule(bundledUrl) Loader->>Import: import(bundledUrl) alt Bundled import succeeds Import-->>Loader: Module loaded Loader-->>Plugin: Hooks initialized else Bundled import fails Import-->>Loader: Error (uncaught!) Note over Plugin,Import: BUG: Error propagates uncaught end end end end Plugin-->>Config: Return hooks array